-
Notifications
You must be signed in to change notification settings - Fork 403
docs(chain): add doctest for min confirmation balance filtering #2007
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for working on this.
I do not think modifying the canonicalization is the right approach.
For example, if we have a min-confirmation-count of 3. We expect that a transaction confirmed at depth 2 should be prioritized over a transaction with no confirmations. With the current state of the PR, this guarantee is nonexistent.
Additionally, different min-confirmation-count values should not influence the Balance::total
value. It should only shift values between *pending,immature,confirmed
.
Idea 1
The simplest solution would be to not do any code changes and provide a doctest showing how we could manipulate the chain_tip
input of TxGraph::[try_]balance
. I.e. with LocalChain
/CheckPoint
s.
let tip = chain.tip();
let target_height = tip.height().saturating_sub(confirmation_threshold);
let target_tip = tip
.range(..=target_height)
.next()
.expect("checkpoint from local chain must have genesis");
// Use `target_tip` as the `chain_tip` input of `TxGraph::[try_]balance`.
For bdk_wallet::Wallet
, we should probably provide a convenience method of sorts so the caller does not need to dig into the internal structures.
Idea 2
We can change the trust_predicate
to be a more general predicate
.
pub fn balance(
/* Other Inputs */
// predicate inputs: (txout_meta, txout, tip_height)
mut predicate: impl FnMut(&OI, FullTxOut<A>, u32) -> BalanceType,
) -> Balance { todo!() }
// Maybe put this in `bdk_core`?
pub enum BalanceType {
Immature,
TrustedPending,
UntrustedPending,
Confirmed,
}
However, this is a breaking change and it makes it more involved to implement the predicate
.
Let me know what you think.
Thanks for the suggestions. I like Idea 1, and will work on implementing this. |
ad19250
to
1bc9534
Compare
//! let confirmation_threshold = 6; | ||
//! let target_height = tip.height().saturating_sub(confirmation_threshold - 1); | ||
//! let target_tip = chain | ||
//! .range(..=target_height) | ||
//! .next() | ||
//! .expect("checkpoint from local chain must have genesis"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a lot of code to achieve this. Maybe we should introduce convenience methods on CheckPoint
/LocalChain
:
impl CheckPoint {
/// Returns the checkpoint at `height` if one exists, otherwise the nearest checkpoint at a
/// lower height.
///
/// This is equivalent to taking the "floor" of `height` over this checkpoint chain.
///
/// Returns `None` if no checkpoint exists at or below the given height.
pub fn floor_at(&self, height: u32) -> Option<Self> {
self.range(..=height).next()
}
/// Returns the checkpoint located a number of heights below this one.
///
/// This is a convenience wrapper for [`CheckPoint::floor_at`], subtracting `to_subtract` from
/// the current height.
///
/// - If a checkpoint exists exactly `offset` heights below, it is returned.
/// - Otherwise, the nearest checkpoint *below that target height* is returned.
///
/// Returns `None` if `to_subtract` is greater than the current height, or if there is no
/// checkpoint at or below the target height.
pub fn floor_below(&self, offset: u32) -> Option<Self> {
self.floor_at(self.height().checked_sub(offset)?)
}
}
Also, I feel like this this example should belong on .balance
(easier to find).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The floor_at
method will be useful for initiating a block-based chain source. I.e. if a certain spk has a birthday at height 100, we can use this method to start scanning from that height. cc. @ValuedMammal
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, I think this example could live in .balance
. Also, I don't see any issue with adding the convenience methods.
Implements #1942.
Description
This PR adds a new doctest demonstrating how to simulate a minimum confirmation threshold for confirmed balance calculations by adjusting the
chain_tip
height passed toTxGraph::balance
.Changelog notice
chain_tip
height.Checklists
All Submissions:
New Features:
Bugfixes: